-
Notifications
You must be signed in to change notification settings - Fork 36
DebugAccumulator (plus tiny bits and pieces) #976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Benchmark Report for Commit 408b951Computer Information
Benchmark Results
|
DynamicPPL.jl documentation for PR #976 is available at: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #976 +/- ##
============================================
- Coverage 82.67% 82.58% -0.09%
============================================
Files 38 38
Lines 4022 4007 -15
============================================
- Hits 3325 3309 -16
- Misses 697 698 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple, more general, things I wanted to ask your opinion on @mhauru:
-
Implementing this was the first time I worked with accs. It was largely a pleasure (I appreciated having good docstrings), but I got a bit confused between
setaccs!!
(replaces all the accumulators) andsetacc!!
(adds to the existing accumulators). Is there a way we could disambiguate? -
My pipe dream for DynamicPPL's folder structure would be something like this:
$ tree
.
├── accs
│ ├── debug.jl
│ ├── interface.jl
│ ├── logprob.jl
│ └── values_as_in_model.jl
├── contexts
│ ├── conditionfix.jl
│ ├── interface.jl
│ └── prefix.jl
├── DynamicPPL.jl
├── model.jl
└── varinfo.jl
(Not all files included.) We kind of have something like this already in that accumulators.jl
is what I call accs/interface.jl
, and default_accumulators.jl
is what I call accs/logprob.jl
(modulo NumProduce), but I'd like to go one step further and use directories. I want to do the same with contexts, and I feel like perhaps you mentioned something similar for varinfos? Shall we maybe put this on the list as the last thing to do before releasing 0.37?
## 0.37.0 | ||
|
||
**Breaking changes** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.37 is turning into a bit of a monster. I'm personally quite happy with this, even a little bit excited! 😄
But I think we might need to start treating it a bit more seriously. I started by separating the changelog into more public vs more internal changes. (For example, most people really don't need to care about accs; even if you're using something like values_as_in_model
, you don't need to care about whether it was implemented using a context or an acc.)
Apart from this, maybe we should probably have a definition of done for 0.37 (ie which PRs/features do we want to get in for that release)? If you agree, then I'll start putting together a checklist on the breaking
PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me the only real question is, do we want the simplifications afforded by accumulators to be in v0.37 or not. Either all of them or most of them. I would like VariableOrderAccumulator, because that's kinda where this whole thing started, getting rid of num_produce
. Haven't thought carefully about what else might be on the fence of whether it's in or out.
I have nothing that would be entirely unrelated to accumulators that I would like to put in v0.37.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very happy with the improvements to the changelist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of accs, it's just VariableOrder left, isn't it? And we could keep it as a default acc in 0.37, and maybe later work out how to make it opt-in for PG only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be just VariableOrder, though I haven't looked at this in a few weeks, so may forget something. We could keep it as default, but I would also consider leaving it out and immediately moving it Turing.jl once it's functional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that we've actually converged to PG on two fronts, one with VariableOrderAcc and removing num_produce
, the other with removing SamplingContext
and the del
flag #982.
If you asked me to be opinionated: I wonder if it may be easier for us to leave all the PG-related stuff to 0.38, partly because 0.37 is getting very big, and partly so that we can compartmentalise PG and non-PG stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really mind v0.37 getting big, when we do PRs one by one, as long as it doesn't start to hold up releasing something of use. I guess it might make the integration work in Turing.jl more painful. Leaving things like removing code that is being added to Turing.jl for v0.38 would make sense, to have one release where it's all in place but not yet gone. Generally not bothered if you want to make an intermediate release. When you say "leave all the PG-related stuff to 0.38", would that include implementing VariableOrderAccumulator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you say "leave all the PG-related stuff to 0.38", would that include implementing VariableOrderAccumulator?
Yup.
src/accumulators.jl
Outdated
# When showing with text/plain, leave out information about the wrapper AccumulatorTuple. | ||
Base.show(io::IO, mime::MIME"text/plain", at::AccumulatorTuple) = show(io, mime, at.nt) | ||
# When showing with text/plain, leave out type information about the wrapper AccumulatorTuple. | ||
function Base.show(io::IO, mime::MIME"text/plain", at::AccumulatorTuple) | ||
print(io, "AccumulatorTuple(") | ||
show(io, mime, at.nt) | ||
print(io, ")") | ||
return nothing | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also opinionated. I like that you can (usually) copy the output of show, paste it into the REPL and have it generate the same object. Happy to revert if you disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that should be the case for show
with no MIME type specified, and this is in fact in Julia docs. However, I view MIME"text/plain"
as a request to be more human-readable and pretty/slick at the expense of completeness and machine-parseability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although show with no MIME defaults to text/plain, doesn't it? So it seems like the same thing to me.
Part of the reason why I'd like to include this is e.g. when trying to debug (say, Enzyme issues) then it printing the same as a NamedTuple feels a bit misleading (I'd have to check typeof
to realise that it is, in fact, not a NamedTuple). I'm not hugely opinionated because I will probably remember, but maybe it might help somebody down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how the method cascade is implemented, but even if you define the MIME"text/plain"
version, the plain call to show
still uses the default implementation:
julia> struct Dada end
julia> Base.show(io::IO, mime::MIME"text/plain", ::Dada) = print(io, "three arg text/plain")
julia> Dada()
three arg text/plain
julia> show(Dada())
Dada()
julia> display(Dada())
three arg text/plain
julia> @show Dada();
Dada() = Dada()
Do stacktraces end up using the display
/three arg show
thing somewhere? Because if yes then I see your point about debugging, and it becomes a question of ease of debugging vs neatness of user-facing output. I was hoping the three arg MIME"text/plain"
would only come into play in the REPL and if one calls display
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stack traces would show the type so it would be alright there. And oh, okay, it seems that it's actually defined something like this:
display(x) = Base.show(stdout, MIME"text/plain"(), x)
Base.show(io, ::MIME"text/plain", x) = Base.show(io, x)
Base.show(x) = Base.show(stdout, x)
I think I still prefer the consistency of it always printing AccumulatorTuple
. Maybe the problem is that I actually do use the user-facing output for debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I have with that is that when someone calls e.g. display(svi)
I take that to mean "give me a human-readable, pretty, not-necessarily-exhaustive summary of what is in this SimpleVarInfo". In which case if it prints out
Transformed SimpleVarInfo((x = -1.0,), AccumulatorTuple((LogPrior = LogPriorAccumulator(0.0), LogLikelihood = LogLikelihoodAccumulator(0.0), NumProduce = NumProduceAccumulator(0))))
I find the word AccumulatorTuple
to be unnecessary bloat that makes the output uglier and harder to read. If you care about AccumulatorTuple
then I presume you also care about things like "is that an Int64 or Int32?" and you should use show(svi)
, which should give you all the details. Really what I would like for display(svi)
to print out might be
Transformed SimpleVarInfo((x = -1.0,), (LogPrior = 0.0, LogLikelihood = 0.0, NumProduce = 0))
though I don't think I ever got to making a nice implementation that would do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with the gist of it, some localised comments.
One change that this brings is that if your context does something weird and e.g. fails to call accumulate_obssume!!
, then nothing will be captured by the DebugAccumulator
. Previously, since DebugContext
arrested the call stack higher up, things like record_pre_tilde_assume!
were being called no matter what. Not sure if this change is a pro or a con.
## 0.37.0 | ||
|
||
**Breaking changes** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me the only real question is, do we want the simplifications afforded by accumulators to be in v0.37 or not. Either all of them or most of them. I would like VariableOrderAccumulator, because that's kinda where this whole thing started, getting rid of num_produce
. Haven't thought carefully about what else might be on the fence of whether it's in or out.
I have nothing that would be entirely unrelated to accumulators that I would like to put in v0.37.
## 0.37.0 | ||
|
||
**Breaking changes** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very happy with the improvements to the changelist.
src/accumulators.jl
Outdated
# When showing with text/plain, leave out information about the wrapper AccumulatorTuple. | ||
Base.show(io::IO, mime::MIME"text/plain", at::AccumulatorTuple) = show(io, mime, at.nt) | ||
# When showing with text/plain, leave out type information about the wrapper AccumulatorTuple. | ||
function Base.show(io::IO, mime::MIME"text/plain", at::AccumulatorTuple) | ||
print(io, "AccumulatorTuple(") | ||
show(io, mime, at.nt) | ||
print(io, ")") | ||
return nothing | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that should be the case for show
with no MIME type specified, and this is in fact in Julia docs. However, I view MIME"text/plain"
as a request to be more human-readable and pretty/slick at the expense of completeness and machine-parseability.
src/simple_varinfo.jl
Outdated
```jldoctest simplevarinfo-general | ||
julia> vi = DynamicPPL.settrans!!(SimpleVarInfo((x = -1.0,)), true) | ||
Transformed SimpleVarInfo((x = -1.0,), (LogPrior = LogPriorAccumulator(0.0), LogLikelihood = LogLikelihoodAccumulator(0.0), NumProduce = NumProduceAccumulator(0))) | ||
Transformed SimpleVarInfo((x = -1.0,), AccumulatorTuple((LogPrior = LogPriorAccumulator(0.0), LogLikelihood = LogLikelihoodAccumulator(0.0), NumProduce = NumProduceAccumulator(0)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was one of the reasons why I liked the simplified MIME"text/plain"
show: To declutter printing out varinfo types.
You now need to explicitly pass a `VarInfo` argument to `check_model` and `check_model_and_trace`. | ||
Previously, these functions would generate a new VarInfo for you (using an optionally provided `rng`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, check_model
signature still says
check_model(model::Model, varinfo::AbstractVarInfo=VarInfo(model); error_on_failure=false)
making varinfo
optional. The keyword arguments I think have changed though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that is true, I didn't realise that. Errrrr, I can't say I like having the VarInfo be optional. In evaluate!!
it isn't optional, and check_model
basically does evaluate!!
with extra steps. I think I will make it mandatory, if that's alright. I don't think anyone uses this directly (it's mostly a pre-sampling thing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that it was semantically neat that to check
a model you only needed to give a model, and then if you wanted to specify more about how that checking of a model is done, that was optional. Not too fussed about it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeahh, agreed. That's the main reason why I was a bit hesitant in my last comment. The single-argument version did require being restrictive in that it would always use SamplingContext though (there was no way to change this). I guess I'll keep it this way now (with both model and varinfo compulsory) but also keep it in mind as one of the areas where we're not fully sure about the best API.
record_pre_tilde_assume!(context, vn, right, vi) | ||
value, vi = DynamicPPL.tilde_assume(childcontext(context), right, vn, vi) | ||
record_post_tilde_assume!(context, vn, right, value, vi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason why pre
and post
were separate? Just wondering if we are losing something in effectively only having post
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only thing that matters is the missing
check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which doesn't matter any more because I removed the logp accumulators.
Indeed that's true. I think it depends on your view of what
That's kind of similar to the other functions in DebugUtils, e.g. IMO I don't think it's its job to catch:
|
Note that
I'd be happy with this sort of restructuring. My only change to your proposal would be that, depending on how varinfo source code gets structured, |
Happy with that (and thus leaving out the missing and context checking). |
I'm a bit annoyed that there isn't a better way for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to consider this done except for the ongoing conversation about display
and show
.
Co-authored-by: Markus Hauru <[email protected]>
We agreed that I should use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Forgot to revert the doctests changes with the show method. Will do that then merge. |
* Bump minor version to 0.37.0 * Accumulators, stage 1 (#885) * Release 0.36 * AbstractPPL 0.11 + change prefixing behaviour (#830) * AbstractPPL 0.11; change prefixing behaviour * Use DynamicPPL.prefix rather than overloading * Remove VarInfo(VarInfo, params) (#870) * Unify `{untyped,typed}_{vector_,}varinfo` constructor functions (#879) * Unify {Untyped,Typed}{Vector,}VarInfo constructors * Update invocations * NTVarInfo * Fix tests * More fixes * Fixes * Fixes * Fixes * Use lowercase functions, don't deprecate VarInfo * Rewrite VarInfo docstring * Fix methods * Fix methods (really) * Draft of accumulators * Fix some variable names * Fix pointwise_logdensities, gut tilde_observe, remove resetlogp!! * Map rather than broadcast Co-authored-by: Tor Erlend Fjelde <[email protected]> * Start documenting accumulators * Use Val{symbols} instead of AccTypes to index * More documentation for accumulators * Link varinfo by default in AD testing utilities; make test suite run on linked varinfos (#890) * Link VarInfo by default * Tweak interface * Fix tests * Fix interface so that callers can inspect results * Document * Fix tests * Fix changelog * Test linked varinfos Closes #891 * Fix docstring + use AbstractFloat * Fix resetlogp!! and type stability for accumulators * Fix type rigidity of LogProbs and NumProduce * Fix uses of getlogp and other assorted issues * setaccs!! nicer interface and logdensity function fixes * Revert back to calling the macro @addlogprob! * Remove a dead test * Clarify a comment * Implement split/combine for PointwiseLogdensityAccumulator * Switch ThreadSafeVarInfo.accs_by_thread to be a tuple * Fix `condition` and `fix` in submodels (#892) * Fix conditioning in submodels * Simplify contextual_isassumption * Add documentation * Fix some tests * Add tests; fix a bunch of nested submodel issues * Fix fix as well * Fix doctests * Add unit tests for new functions * Add changelog entry * Update changelog Co-authored-by: Hong Ge <[email protected]> * Finish docs * Add a test for conditioning submodel via arguments * Clean new tests up a bit * Fix for VarNames with non-identity lenses * Apply suggestions from code review Co-authored-by: Markus Hauru <[email protected]> * Apply suggestions from code review * Make PrefixContext contain a varname rather than symbol (#896) --------- Co-authored-by: Hong Ge <[email protected]> Co-authored-by: Markus Hauru <[email protected]> * Revert ThreadSafeVarInfo back to Vectors and fix some AD type casting in (Simple)VarInfo * Improve accumulator docs * Add test/accumulators.jl * Docs fixes * Various small fixes * Make DynamicTransformation not use accumulators other than LogPrior * Fix variable order and name of map_accumulator!! * Typo fixing * Small improvement to ThreadSafeVarInfo * Fix demo_dot_assume_observe_submodel prefixing * Typo fixing * Miscellaneous small fixes * HISTORY entry and more miscellanea * Add more tests for accumulators * Improve accumulators docstrings * Fix a typo * Expand HISTORY entry * Add accumulators to API docs * Remove unexported functions from API docs * Add NamedTuple methods for get/set/acclogp * Fix setlogp!! with single scalar to error * Export AbstractAccumulator, fix a docs typo * Apply suggestions from code review Co-authored-by: Penelope Yong <[email protected]> * Rename LogPrior -> LogPriorAccumulator, and Likelihood and NumProduce * Type bound log prob accumulators with T<:Real * Add @addlogprior! and @addloglikelihood! * Apply suggestions from code review Co-authored-by: Penelope Yong <[email protected]> * Move default accumulators to default_accumulators.jl * Fix some tests * Introduce default_accumulators() * Go back to only having @addlogprob! * Fix tilde_observe!! prefixing * Fix default_accumulators internal type * Make unflatten more type stable, and add a test for it * Always print all benchmark results * Move NumProduce VI functions to abstract_varinfo.jl --------- Co-authored-by: Penelope Yong <[email protected]> Co-authored-by: Tor Erlend Fjelde <[email protected]> Co-authored-by: Hong Ge <[email protected]> * Replace PriorExtractorContext with PriorDistributionAccumulator (#907) * Implement values_as_in_model using an accumulator (#908) * Implement values_as_in_model using an accumulator * Make make_varname_expression a function * Refuse to combine ValuesAsInModelAccumulators with different include_colon_eqs * Fix nested context test * Bump DynamicPPL versions * Fix merge (1) * Add benchmark Pkg source * [no ci] Don't need to dev again * Disable use_closure for ReverseDiff * Revert "Disable use_closure for ReverseDiff" This reverts commit 3cb47cd. * Fix LogDensityAt struct * Try not duplicating * Update comment pointing to closure benchmarks * Remove `context` from model evaluation (use `model.context` instead) (#952) * Change `evaluate!!` API, add `sample!!` * Fix literally everything else that I broke * Fix some docstrings * fix ForwardDiffExt (look, multiple dispatch bad...) * Changelog * fix a test * Fix docstrings * use `sample!!` * Fix a couple more cases * Globally rename `sample!!` -> `evaluate_and_sample!!`, add changelog warning * Mark function as Const for Enzyme tests (#957) * Move submodel code to submodel.jl; remove `@submodel` (#959) * Move submodel code to submodel.jl * Remove `@submodel` * Fix missing field tests for 1.12 (#961) * Remove 3-argument `{_,}evaluate!!`; clean up submodel code (#960) * Clean up submodel code, remove 3-arg `_evaluate!!` * Remove 3-argument `evaluate!!` as well * Update changelog * Improve submodel error message * Fix doctest * Add error hint for three-argument evaluate!! * Improve API for AD testing (#964) * Rework API for AD testing * Fix test * Add `rng` keyword argument * Use atol and rtol * remove unbound type parameter (?) * Don't need to do elementwise check * Update changelog * Fix typo * DebugAccumulator (plus tiny bits and pieces) (#976) * DebugContext -> DebugAccumulator * Changelog * Force `conditioned` to return a dict * fix conditioned implementation * revert `conditioned` bugfix (will merge this to main instead) * fix show * Fix doctests * fix doctests 2 * Make VarInfo actually mandatory in check_model * Re-implement `missing` check * Revert `combine` signature in docstring * Revert changes to `Base.show` on AccumulatorTuple * Add TODO comment about VariableOrderAccumulator Co-authored-by: Markus Hauru <[email protected]> * Fix doctests --------- Co-authored-by: Markus Hauru <[email protected]> * VariableOrderAccumulator (#940) * Turn NumProduceAccumulator into VariableOrderAccumulator * Add comparison methods * Make VariableOrderAccumulator use regular Dict * Use copy rather than deepcopy for accumulators * Minor docstring touchup * Remove unnecessary use of NumProduceAccumulator * Fix split(VariableOrderAccumulator) * Remove NumProduceAcc from Debug * Fix set_retained_vns_del! --------- Co-authored-by: Penelope Yong <[email protected]> * Accumulators stage 2 (#925) * Give LogDensityFunction the getlogdensity field * Allow missing LogPriorAccumulator when linking * Trim whitespace * Run formatter * Fix a few typos * Fix comma -> semicolon * Fix `LogDensityAt` invocation * Fix one last test * Fix tests --------- Co-authored-by: Penelope Yong <[email protected]> * Implement more consistent tracking of logp components via `LogJacobianAccumulator` (#998) * logjac accumulator * Fix tests * Fix a whole bunch of stuff * Fix final tests * Fix docs * Fix docs/doctests * Fix maths in LogJacobianAccumulator docstring * Twiddle with a comment * Add changelog * Fix accumulator docstring * logJ -> logjac * Fix logjac accumulation for StaticTransformation * Fix behaviour of `set_retained_vns_del!` for `num_produce == 0` (#1000) * `InitContext`, part 2 - Move `hasvalue` and `getvalue` to AbstractPPL; enforce key type of `AbstractDict` (#980) * point to unmerged AbstractPPL branch * Remove code that was moved to AbstractPPL * Remove Dictionaries with Any key type * Fix bad merge conflict resolution * Fix doctests * Point to [email protected] This reverts commit 709dc9e. * Fix doctests * Fix docs AbstractPPL bound * Remove stray `Pkg.update()` * Accumulator miscellanea: Subset, merge, acclogp, and LogProbAccumulator (#999) * logjac accumulator * Fix tests * Fix a whole bunch of stuff * Fix final tests * Fix docs * Fix docs/doctests * Fix maths in LogJacobianAccumulator docstring * Twiddle with a comment * Add changelog * Simplify accs with LogProbAccumulator * Replace + with accumulate for LogProbAccs * Introduce merge and subset for accs * Improve acc tests * Fix docstring typo. Co-authored-by: Penelope Yong <[email protected]> * Fix merge --------- Co-authored-by: Penelope Yong <[email protected]> * Minor tweak to changelog wording --------- Co-authored-by: Penelope Yong <[email protected]> Co-authored-by: Tor Erlend Fjelde <[email protected]> Co-authored-by: Hong Ge <[email protected]>
Closes #974.
My comments in review.